We should be sure to have a test that has both w/ and without type:"json" and check if they are properly resolved seems per last discussion they would be the same if both resolve to same type. Also should have one that races w/ unknown type.

@bmeck any chance you would be able to write the edge case test you mentioned in the other PR and send a commit to my branch? I'm not sure I understand exactly what would be the expected behavior, so tests would be very helpful.

@bmeck I've implemented in ece7a7d...733deb8 the suggested cache implementation you mentioned in #37375 (comment), PTAL. I've used a Map<URLString, NullPrototypeObject<AssertTypeStringOrUndefined, ModuleJob>> instead of a Map<URLString, Map<AssertTypeStringOrNull, ModuleJob>>, that seemed easier to implement but there might be a good reason to prefer your suggestion so let me know.

If the PR only supports loading JSON with type and fails when type is missing, my block can be ignored; but discussion is mixed between these 2 PRs on that.

It fails in both cases, or succeed in both cases when loaded with --experimental-json-modules flag.

@aduh95 my concern is that it shouldn’t be possible to import json with the assertion unless it’s possible to load it without the assertion. Sounds like this PR doesn’t create that scenario.

@GeoffreyBooth I removed the assertionless syntax from the docs, as you requested. Removing it from the tests would not be possible as I explained above, we can discuss adding a --experimental-import-non-javascript-without-assertion CLI flag later (PR's welcome). Approving this PR doesn't mean approval for JSON modules unflagging to be clear (in whichever form), and the discussion can continue in the other PR.

@targos @aduh95

In my mind, the two blocks are about two separate things:

• On module: Unflag JSON modules #37375, I think that PR shouldn’t land (we shouldn’t unflag JSON modules) until the assertion syntax is required to import JSON.

• On this PR, there are two things being added: support for the assertion syntax, and a whole bunch of tests and logic to handle what to do when there are separate import statements for the same specifier where one import has an assertion and one does not. This is what @aduh95 proposes putting behind a separate --experimental-import-non-javascript-without-assertion flag.

Personally I think the code that would go under --experimental-import-non-javascript-without-assertion is unnecessary complexity, both for Node core and for what it means for loaders. I just don’t think it should be added to Node, behind a flag or not. This PR doesn’t need to add support for the assertion syntax and add support for two forms; it could just convert the current support for import of JSON into needing the assertion and stop there. So we’re not in a chicken-and-egg scenario exactly, I don’t think.

So I think the path forward is to remove the support for two forms from this PR, leaving only the assertion form, and then this PR can land; and then #37375 can land and we unflag JSON modules. If @aduh95 or others still want to propose the additional support for assertionless JSON imports, that could be opened as a new PR to add that and it can be debated on its own merits.

this PR [adds] a whole bunch of tests and logic to handle what to do when there are separate import statements for the same specifier where one import has an assertion and one does not.

Note that this is a spec recommendation (https://tc39.es/proposal-import-assertions/#sec-hostresolveimportedmodule):

It is recommended but not required that implementations additionally conform to the following stronger constraint: each time this operation is called with a specific referencingScriptOrModule, moduleRequest.[[Specifier]] pair as arguments it must return the same Module Record instance if it completes normally.

Removing the possibility of this happening is worth discussing, but I think that's out of the scope of this PR.

Neither JSON modules nor import assertions should be unflagged until you can import JSON without an assertion, since that scenario being possible is the only reason EITHER FEATURE EXISTS.

When this was discussed in TC39 we were under the impression Node.js had reached consensus about this plan (that node would allow JSON modules to be imported both with and without the assertion, while browsers would mandate the assertion). This was communicated to the committee by the proposal champions and various node representatives. It is frustrating to see this consensus has been lost, after years of effort with TC39 and browser implementations. If this block persists, I think we will need to rethink how Node.js and TC39 interacts because of how important it is to build trust and good faith between these bodies in order to advance proposals.

@ljharb those talks which I was present for and participated in were to my recollection about allowing the assertion to be omitted, not enforcing that a host such as node must implement it in a way that it is omitted. You seem to imply that discussion was done in bad faith which is not what I see or have participated in. If you want to use TC39 as a lever to force Node to implement something I am a bit worried. The different opinions of hosts and standards evolve over time for one point, and another is that the Node collaborators were not universally in those discussions and framing it as if all the consensus was only done outside of the typical way that consensus is formed for Node features is concerning.

@bmeck the discussions were done with the explicitly stated assumption that node would allow the assertion to be omitted (of course, not that the spec would mandate anyone omit it, only permit it). I do not believe any of this was done in bad faith, including the block here.

It seems that what happened is that I, at least, did not realize the potential for a collaborator to block at the last minute. I only provided consensus for advancement of both JSON Modules as well as Import Assertions, at any stage, because I believed node would allow omitting the assertion. Had I realized this block was a possible outcome, I would have not allowed either proposal to advance without at least a TSC decision beforehand to omit the assertion.

Separately, I am highly concerned about a block on the philosophical grounds of "node must not have a feature that a browser lacks", and I think that would be a very worrying precedent for node.

It's clear I've allowed my frustration and fear about this topic to drive me to make very strong statements in this thread, and I apologize that at times that means I've overstepped, or too-strongly characterized history or the opinions of others, and additionally that I've led some to believe I'm implying bad faith in the past or now - which was never my intention.

@ljharb even with discussions about wanting to allow the assertion-less form, concerns about non-philosophical things are popping up as I'm trying to figure out the right way to let user provided loaders handle the races that occur if both forms are supported. Such a thing likely wouldn't have shown up until we looked at how to integrate with Loaders and after stage 3 was achieved anyway.

@bmeck your technical concerns are valid and do not give me the same level of concern. I believe they can be overcome, and if they can't be, then that is a different and concrete sort of issue than a philosophical concern.

Based on TSC conversation today the TSC felt comfortable with separating landing this PR only supporting the form with the assertion and doing a follow up discussion to determine the viability of shipping the version without the assertion afterwards. This would require changes to the current implementation

The thought here was that, at least in my impression, there would not be a world where we would ship ONLY the assertionless approach and not support the assertion approach as this would be explicitly breaking code that will work in the browser.

I have a question for folks who participated in the TC39 discussions about this proposal: is it fair to assume that JSON modules will not be the only type of module that will be permitted to be imported both with and without an assertion? My thinking when working on this PR was that other module formats would be added to the list (maybe WASM, JavaScript even), and that JSON modules just happen to be the first.
If JSON modules are suppose to be one-off and no other module formats will be forced to be imported using only one or the other, I think I get why @GeoffreyBooth thinks the added complexity is not worth it. If on the other hand, we think that other modules format will be concerned, then the added tests and logic in this PR are necessary for Node.js to be future-proof imho.

Wasm thread on import assertion requirement seems as yet undecided here - WebAssembly/esm-integration#42. I'm glad caution is being taken and consideration is being given to platform support, but given this PR doesn't ship anything unflagged I'm glad the TSC came to the conclusion it seems good to ship.

@aduh95 the intention was for the burden of the assertion to only be required when it was necessary, which for filesystem JSON modules, it’s not. The same would be true of wasm modules, presumably, since they have a file extension, so I’d expect those assertions to be optional as well.

If JSON modules are suppose to be one-off and no other module formats will be forced to be imported using only one or the other, I think I get why @GeoffreyBooth thinks the added complexity is not worth it. If on the other hand, we think that other modules format will be concerned, then the added tests and logic in this PR are necessary for Node.js to be future-proof imho.

As long as the assertionless form is optional for any future formats, then we won’t need to support two syntaxes for the same format. Assertionless could be only for JavaScript and assertion-required could be every non-JavaScript format. Node wouldn’t need to support two syntaxes for the same format, for any format.

If the assertion becomes allowed for JavaScript, then we would need to support two forms. Is there a proposal for that?

It’s much simpler from a maintenance burden perspective and from a loaders development DX perspective to only support one syntax per file type. I don’t want to add “assertion or assertionless for the same file type” support unless and until we’re required to.

Edit: I suppose another option that the standards bodies could take would be to add new formats as assertionless-only, similar to JavaScript (like maybe that’s how Wasm goes). And then we’d be in the same position, where JavaScript and Wasm are assertionless and JSON (and possibly other formats like CSS or HTML) are assertion-required; and if/when the day comes that any format can be more than one syntax is the day that we add support for two syntaxes for the same format.

I think having an actual meeting rather than PR comments would help clear things up as these discussions have dragged on long enough that my brain is having trouble understanding the actual state of the PR or potential eventual state of the PR

Would it make sense to discuss that in a @nodejs/loaders meeting? I could join as a guest if that's OK. Or we could schedule a call outside of the usual WG meetings, I've never done that myself so I suppose we'd need a volunteer to set that up for us.

Would it make sense to discuss that in a @nodejs/loaders meeting? I could join as a guest if that’s OK. Or we could schedule a call outside of the usual WG meetings, I’ve never done that myself so I suppose we’d need a volunteer to set that up for us.

I added the loaders-agenda flag to #40210. The next meeting was already scheduled for Tuesday (nodejs/loaders#34) and I think the plan was to discuss chaining unless that’s not ready; but that will probably take the entire time if it is.

I’m not sure we need a meeting just yet? Per #39921 (comment) I think if @aduh95 or someone else is willing to open a PR that simply lands the assertion syntax for JSON modules without the additional support of making the assertion syntax optional, such a PR can land and we can unflag JSON modules. Then either this PR or #40210 would only consist of making the assertion syntax optional, which would merit a meeting for sure. But I’m not sure we need discussion around that before we’ve landed the other two PRs first?

Converting to draft to avoid confusion with #40250, which is the version of this being actively developed.

Closing as #40250 is the version under active development.